feat(mobile): add integration with gateway backend for notifications feature #4878
Conversation
Branch preview✅ Deploy successful! Storybook: |
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Accept: 'application/json', | ||
| 'Set-Cookie': 'HttpOnly;Secure;SameSite=None', | ||
| }, |
There was a problem hiding this comment.
Is this block necessary? Especially the set-cookie header is a response header. It shouldn't do anything here?
There was a problem hiding this comment.
Yes, without this all requests fails due unauthorized.
| dispatch(toggleAppNotifications(!isAppNotificationEnabled)) | ||
| if (!isAppNotificationEnabled) { | ||
| await deleteDelegate() |
There was a problem hiding this comment.
what happens if for whatever reason this fails? Is the user still going to receive push notifications?
There was a problem hiding this comment.
changed the order of the calls to avoid disable redux without the confirmation of BE.
There was a problem hiding this comment.
@Jonathansoufer I have my doubts that this if(!error) check is goin to work reliably. the useDelegateKey hook returns an error. here you await the deleteDelegate and thje deleteDelegate function might set the error, but if does the notificationContainer needs to rerender. I'm not sure how the flow of the code is going to be. Isn't delegate always be null, since you are in the callback and at the moment the callback has started it is null 🤷
Wouldn't it be easier of the deleteDelegate and createDelegate return the error or throw?
| reducers: { | ||
| addDelegatedAddress: (state, action: PayloadAction<{ delegatedAddress: Address; safes: SafeInfo[] }>) => { | ||
| const { delegatedAddress, safes } = action.payload | ||
| state.delegatedSafes[delegatedAddress] = { safes } |
There was a problem hiding this comment.
I couldn't run the branch to inspect the final form of the state, but as far as I understand we end up with
delegated.delegatedSafes
Can't we just have a slice delegates and then have a mapping of delegatedAddress to safes? Why do we need the extra nesting?
There was a problem hiding this comment.
Good catch. Changed.
| updateDelegatedAddress: (state, action: PayloadAction<{ delegatedAddress: Address; safes: SafeInfo[] }>) => { | ||
| const { delegatedAddress, safes } = action.payload | ||
|
|
||
| state.delegatedSafes[delegatedAddress] = { ...safes, safes } |
There was a problem hiding this comment.
We should feel lucky that Immer is part of redux-toolkit 😄
https://redux-toolkit.js.org/usage/immer-reducers
|
@Jonathansoufer - I built the app locally and installed on my phone, but when I click on "turn on" notificatoins and navigate to the settings I don't have any notifications to choose from? The permission is not in the list? |
| @@ -1,7 +1,9 @@ | |||
| //@ts-ignore | |||
| globalThis.RNFB_SILENCE_MODULAR_DEPRECATION_WARNINGS = true | |||
There was a problem hiding this comment.
I think we better move this to the entry point of the application. It's a bit weird to have this global var first in the FCMService. It seems to be a global thing that react-native-firebase "needs".
| if (delegatedAccountType === DELEGATED_ACCOUNT_TYPE.REGULAR) { | ||
| await authVerifyV1({ | ||
| siweDto: { | ||
| message, | ||
| signature, | ||
| }, | ||
| }) | ||
| } else { | ||
| delegatesPostDelegateV2({ | ||
| chainId, | ||
| createDelegateDto: { | ||
| safe: safeAddress, | ||
| delegator: signer.address, | ||
| delegate: delegatedAccount.address, | ||
| signature, | ||
| label: DELEGATED_ACCOUNT_TYPE.OWNER, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
I don't fully understand this block. Why does a regular user need to authenticate, but an owner doesn't?
| if (Platform.OS === 'ios') { | ||
| Linking.openSettings() | ||
| } else { | ||
| notifee.openNotificationSettings() |
There was a problem hiding this comment.
why is it necessary that we use the notifee openNotificationSettings? isn't Linking.openSettings() also working on android?
What it solves
Resolves #
How this PR fixes it
How to test it
Screenshots
Checklist